-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improving shard handling and testing #3
Conversation
Co-authored-by: Kartik Singhal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides removing the lingering print statement, looks good!
self._pending_commands: dict[UnitID, list[Command]] = {} | ||
self._shards: list[Shard] = [] | ||
print(f"Sharder created for circuit {self._circuit}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want prints vs. some other form of logging? I'm ok with it for first cut merging PR in, but I imagine we won't want to keep these prints as is.
|
||
# Handle dependency calculations | ||
depends_upon: set[int] = set() | ||
for shard in self._shards: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if dependency creation should be a self contained method. The overall _build_shard
method isn't too long but this could be a natural part to break down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments on shard.py
but overall I'm in favor of merging for now to facilitate end to end progress and we can clean things up later.
Shard class being filled out, including some testing for basic use cases.